Skip to content

Add extra metrics#175

Merged
brucetony merged 9 commits into
mainfrom
150-add-extra-metrics
May 6, 2026
Merged

Add extra metrics#175
brucetony merged 9 commits into
mainfrom
150-add-extra-metrics

Conversation

@brucetony
Copy link
Copy Markdown
Collaborator

@brucetony brucetony commented Apr 30, 2026

Summary by CodeRabbit

Release Notes

  • New Features

    • Added admin role-based access control for enhanced permission management
    • Expanded log querying with date range filtering and pagination support
    • New analysis log history endpoint for tracking per-run logs
    • Support for raw log query execution
    • API request counting and response statistics tracking
    • Query parameters for flexible analysis node filtering
  • Improvements

    • Enhanced Victoria Logs configuration validation to prevent service errors

@brucetony brucetony linked an issue Apr 30, 2026 that may be closed by this pull request
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 30, 2026

Warning

Rate limit exceeded

@brucetony has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 40 minutes and 58 seconds before requesting another review.

To continue reviewing without waiting, purchase usage credits in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ee139a65-4371-47df-b34e-f4c964ea2ec9

📥 Commits

Reviewing files that changed from the base of the PR and between a27cf6c and 445c86d.

📒 Files selected for processing (2)
  • hub_adapter/auth.py
  • hub_adapter/routers/logs.py
📝 Walkthrough

Walkthrough

Adds admin role-based access control via a new require_admin_role dependency, introduces a Victoria Logs precondition decorator, expands the logs router with raw LogQL query execution, per-container log retrieval, analysis log history, and API request counting, and extends related schemas with new data models for network stats and query responses.

Changes

Logging, Monitoring, and Admin Authorization Expansion

Layer / File(s) Summary
Schema & Data Models
hub_adapter/schemas/logs.py
New models added: NetStatRun, NetStatTotal, NetStatResponse for network statistics; LogQLQueryRequest, LogQLQueryResponse for raw LogQL queries; ApiRequestCountResponse for API request aggregation. Meta.offset updated to optional. Event tracking keys added for new endpoints.
Core Utilities & Guards
hub_adapter/auth.py, hub_adapter/errors.py
require_admin_role() added to enforce ADMIN_ROLE via nested token claims. require_victoria_logs() decorator added to guard endpoints with HTTP 503 when VictoriaLogs URL is unconfigured.
Log Query Helpers
hub_adapter/routers/logs.py (lines 62–171, 360–370)
Internal helpers added: _execute_raw_query() for raw LogQL execution, _query_pod_logs() for container-specific logs with date range and pagination, _parse_netstats_container() to extract analysis ID and run number from container names.
Endpoint Implementation
hub_adapter/routers/logs.py (lines 216–357)
get_events() updated to async with expanded filters and guarded by @require_victoria_logs. get_analysis_logs() refactored to use _query_pod_logs() for per-container retrieval. get_analysis_log_history() added to aggregate logs across runs. New raw_log_query() and get_api_requests() endpoints implemented for LogQL and API request counting.
Router Integration
hub_adapter/routers/hub.py
list_specific_analysis_node() updated to accept and forward query parameters via new _parse_query_params dependency.
Test Coverage
tests/router_tests/routes.py, tests/router_tests/test_logs.py, tests/test_auth.py, tests/test_user_settings.py
Route configuration assertions extended with three new log endpoints. Comprehensive test suite added for API request counting, raw log queries, and analysis log history. Dependency injection pattern migrated from local get_settings to hub_adapter.dependencies.get_settings. New tests added for require_admin_role() with admin, non-admin, and unconfigured scenarios. User settings operational error tests updated to reflect error source change.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

  • PrivateAIM/node-hub-api-adapter#149: Introduces role claim configuration settings (role_claim_name, admin_role) that are directly consumed by the new require_admin_role() function added in this PR.

Poem

🐰 Hops through the logs, what a delight!
Raw queries now glow in moonlight,
Admin gates guard the sacred ground,
Victoria's voice makes not a sound,
Network stats dance, requests align—
Monitoring blooms in every line! 🌿✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Add extra metrics' is overly vague and does not meaningfully describe the specific changes. The PR actually adds multiple features including admin role enforcement, Victoria Logs integration, query parameter support, and extensive log querying/metrics endpoints. Revise the title to be more specific and descriptive, such as 'Add metrics endpoints and log querying capabilities' or 'Implement admin RBAC and Victoria Logs integration for metrics.'
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 95.92% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 150-add-extra-metrics

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@brucetony
Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 6, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@hub_adapter/auth.py`:
- Around line 270-288: The RBAC check currently returns verified_token when
role_claim_name is set but admin_role is empty, allowing unintended access;
update the check in the block that handles role_claim_name/parsed_claim (the
code that reads role_claim_name, admin_role and returns verified_token) to fail
closed by raising an HTTPException when role_claim_name is truthy but admin_role
is falsy (e.g., raise HTTPException(status_code=500 or 400 with a clear message
about misconfiguration and service "Auth")). Place this validation before
parsing the claim keys so any partial RBAC config triggers an error instead of
bypassing admin role enforcement.

In `@hub_adapter/routers/logs.py`:
- Around line 389-439: The handler currently applies the incoming limit to the
raw _execute_raw_query call so only up to limit raw log rows are fetched before
aggregating into totals (logsql_query/params), which truncates
bytes_in/bytes_out when an analysis produces more rows; change the logic to page
through all raw rows before summing: use count_logs(base_query, params) to get
total, then loop calls to _execute_raw_query with offset/limit (or repeatedly
advance a start/offset parameter) until you've fetched total rows, accumulate
into totals (the totals dict / NetStatRun population) and only then compute meta
and return — alternatively, if you prefer pagination, remove limit from the raw
query and instead paginate the final grouped list (totals) after aggregation;
update usage around logsql_query, params, _execute_raw_query, count_logs, and
totals to implement one of these fixes.
- Around line 216-217: The async endpoint handlers (get_events,
get_analysis_logs, get_analysis_log_history, get_netstats, raw_log_query,
get_api_requests) are calling synchronous helpers (count_logs, query_logs,
_query_pod_logs, _get_analysis_container_names, _execute_raw_query) that use
httpx.Client and block the event loop; convert those helpers to async functions
that use httpx.AsyncClient and await the network calls, then update callers to
await the new async helpers (or alternatively make the handlers normal def so
FastAPI runs them in the threadpool). Specifically: change count_logs,
query_logs, _query_pod_logs, _get_analysis_container_names, _execute_raw_query
to async, replace httpx.Client() with httpx.AsyncClient() and await
.get/post/.stream as appropriate, update all handler calls (e.g., inside
get_events, get_analysis_logs, get_analysis_log_history, get_netstats,
raw_log_query, get_api_requests) to await the helpers, and ensure
require_victoria_logs compatibility and tests pass.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 03e89bf1-fa8f-42b9-911b-b7dd2eabdae7

📥 Commits

Reviewing files that changed from the base of the PR and between d6bfad5 and a27cf6c.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (9)
  • hub_adapter/auth.py
  • hub_adapter/errors.py
  • hub_adapter/routers/hub.py
  • hub_adapter/routers/logs.py
  • hub_adapter/schemas/logs.py
  • tests/router_tests/routes.py
  • tests/router_tests/test_logs.py
  • tests/test_auth.py
  • tests/test_user_settings.py

Comment thread hub_adapter/auth.py Outdated
Comment thread hub_adapter/routers/logs.py
Comment thread hub_adapter/routers/logs.py Outdated
@brucetony brucetony merged commit bffe5cc into main May 6, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add extra metrics

1 participant